Conversation
Summary of ChangesHello @wlgusqkr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 Firebase Cloud Messaging (FCM) 토큰 관리 시스템을 개선하고 확장합니다. 주요 변경 사항은 디바이스 모델 정보를 포함하는 새로운 FCM 토큰 업데이트 API (V2)를 도입한 것입니다. 또한, FCM 메시징 로직을 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
FCM V2 기능 추가와 관련된 변경 사항을 리뷰했습니다. 전반적으로 좋은 변경이라고 생각합니다.
FirebaseMessaging의 정적 호출을FirebaseMessagingClient컴포넌트로 분리하여 테스트 용이성을 높였습니다.deleteAllByFcmTokenIn을 JPQL을 사용한 bulk delete로 변경하여 성능을 개선했습니다.- 기능 추가와 함께 꼼꼼한 테스트 코드를 작성해주셨습니다.
다만 FCMService의 토큰 업데이트 로직에 몇 가지 개선점을 발견하여 코멘트를 남겼습니다.
updateFcmToken메소드에 토큰 값을 비교하는 로직 버그가 있어 수정이 필요합니다.updateFcmToken과updateFcmTokenV2간에 코드 중복이 있어 리팩토링을 권장합니다.- 새로 추가된
updateFcmTokenV2메소드는 사용자가 여러 기기를 사용할 경우를 대비한 설계 개선이 필요해 보입니다.
자세한 내용은 각 파일의 리뷰 코멘트를 참고해주세요.
| List<FCM_Token> FCM_Tokens = fcmRepository.findByUser(user); | ||
|
|
||
| fcmToken.update(SFcmToken); | ||
| if(FCM_Tokens.isEmpty()){ | ||
| FCM_Token newToken = FCM_Token.create(user, requestToken, "unknown"); | ||
| fcmRepository.save(newToken); | ||
| } else { | ||
| FCM_Token existingToken = FCM_Tokens.get(0); | ||
| if (!existingToken.equals(requestToken)) { | ||
| existingToken.updateToken(requestToken); | ||
| } | ||
| } |
There was a problem hiding this comment.
updateFcmToken 메소드에 몇 가지 개선점이 있습니다.
-
[Critical] 149행의
!existingToken.equals(requestToken)비교 로직에 버그가 있습니다.existingToken은FCM_Token객체이고requestToken은String이므로,Object.equals()는 항상false를 반환합니다. 따라서 토큰이 갱신되어야 할 때 갱신되지 않습니다.existingToken.getFcmToken().equals(requestToken)으로 토큰 문자열 값을 직접 비교해야 합니다. -
[Medium] 142행의
FCM_Tokens변수명은 Java 네이밍 컨벤션(camelCase)에 따라fcmTokens로 수정하는 것이 좋습니다.
| List<FCM_Token> FCM_Tokens = fcmRepository.findByUser(user); | |
| fcmToken.update(SFcmToken); | |
| if(FCM_Tokens.isEmpty()){ | |
| FCM_Token newToken = FCM_Token.create(user, requestToken, "unknown"); | |
| fcmRepository.save(newToken); | |
| } else { | |
| FCM_Token existingToken = FCM_Tokens.get(0); | |
| if (!existingToken.equals(requestToken)) { | |
| existingToken.updateToken(requestToken); | |
| } | |
| } | |
| List<FCM_Token> fcmTokens = fcmRepository.findByUser(user); | |
| if(fcmTokens.isEmpty()){ | |
| FCM_Token newToken = FCM_Token.create(user, requestToken, "unknown"); | |
| fcmRepository.save(newToken); | |
| } else { | |
| FCM_Token existingToken = fcmTokens.get(0); | |
| if (!existingToken.getFcmToken().equals(requestToken)) { | |
| existingToken.updateToken(requestToken); | |
| } | |
| } |
| List<FCM_Token> fcmTokens = fcmRepository.findByUser(user); | ||
|
|
||
| if (fcmTokens.isEmpty()) { | ||
| FCM_Token newToken = FCM_Token.create(user, requestToken, model); | ||
| fcmRepository.save(newToken); | ||
| } else { | ||
| FCM_Token existingToken = fcmTokens.get(0); | ||
| if (!existingToken.getFcmToken().equals(requestToken)) { | ||
| existingToken.updateToken(requestToken); | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 updateFcmTokenV2 메소드는 사용자가 여러 기기를 사용하는 경우를 올바르게 처리하지 못할 수 있습니다.
fcmRepository.findByUser(user)로 모든 토큰을 가져온 후, fcmTokens.get(0)으로 리스트의 첫 번째 토큰만 가져와서 업데이트하고 있습니다. 만약 사용자가 여러 기기(예: 아이폰, 아이패드)를 사용한다면, 이 로직은 임의의 기기 토큰을 갱신하게 되어 다른 기기의 푸시 알림이 끊길 수 있습니다.
또한, 기존 토큰이 존재할 경우 토큰 값만 업데이트하고 model 정보는 업데이트하지 않습니다. 사용자가 기기를 변경했을 때 model 정보가 갱신되지 않는 문제가 있습니다.
이 로직은 사용자가 단 하나의 기기만 사용한다고 가정하는 것으로 보입니다. 여러 기기 지원이 필요하다면 아래와 같은 방식으로 재설계하는 것을 고려해 보세요.
(user, model)을 기반으로 토큰을 찾아 업데이트하거나,(user, fcmToken)으로 기존 토큰이 있는지 확인하여 없다면 새로 추가하는 로직
| @Transactional | ||
| public void updateFcmToken(String userUuid, String deviceSecret, String SFcmToken) { | ||
| public void updateFcmToken(String userUuid, String deviceSecret, String requestToken) { | ||
|
|
||
| User user = headerAuthValidator.validateAndGetUser(userUuid, deviceSecret); | ||
|
|
||
| FCM_Token fcmToken = fcmRepository.findByUserId(user.getId()); | ||
| List<FCM_Token> FCM_Tokens = fcmRepository.findByUser(user); | ||
|
|
||
| fcmToken.update(SFcmToken); | ||
| if(FCM_Tokens.isEmpty()){ | ||
| FCM_Token newToken = FCM_Token.create(user, requestToken, "unknown"); | ||
| fcmRepository.save(newToken); | ||
| } else { | ||
| FCM_Token existingToken = FCM_Tokens.get(0); | ||
| if (!existingToken.equals(requestToken)) { | ||
| existingToken.updateToken(requestToken); | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| @Transactional | ||
| public void updateFcmTokenV2(String userUuid, String deviceSecret, String requestToken, String model) { | ||
|
|
||
| User user = headerAuthValidator.validateAndGetUser(userUuid, deviceSecret); | ||
|
|
||
| List<FCM_Token> fcmTokens = fcmRepository.findByUser(user); | ||
|
|
||
| if (fcmTokens.isEmpty()) { | ||
| FCM_Token newToken = FCM_Token.create(user, requestToken, model); | ||
| fcmRepository.save(newToken); | ||
| } else { | ||
| FCM_Token existingToken = fcmTokens.get(0); | ||
| if (!existingToken.getFcmToken().equals(requestToken)) { | ||
| existingToken.updateToken(requestToken); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
updateFcmToken 메소드와 updateFcmTokenV2 메소드의 로직이 거의 동일하여 코드 중복이 발생하고 있습니다. model 파라미터를 받는 부분을 제외하면 로직이 같으므로, 하나의 private 메소드로 추출하여 중복을 제거하고 코드 유지보수성을 높이는 것을 권장합니다.
예시:
@Transactional
public void updateFcmToken(String userUuid, String deviceSecret, String requestToken) {
User user = headerAuthValidator.validateAndGetUser(userUuid, deviceSecret);
upsertFcmToken(user, requestToken, "unknown");
}
@Transactional
public void updateFcmTokenV2(String userUuid, String deviceSecret, String requestToken, String model) {
User user = headerAuthValidator.validateAndGetUser(userUuid, deviceSecret);
upsertFcmToken(user, requestToken, model);
}
private void upsertFcmToken(User user, String requestToken, String model) {
// 현재 updateFcmTokenV2의 로직을 여기에 구현
List<FCM_Token> fcmTokens = fcmRepository.findByUser(user);
if (fcmTokens.isEmpty()) {
FCM_Token newToken = FCM_Token.create(user, requestToken, model);
fcmRepository.save(newToken);
} else {
FCM_Token existingToken = fcmTokens.get(0);
if (!existingToken.getFcmToken().equals(requestToken)) {
existingToken.updateToken(requestToken);
}
}
}
Test Results24 tests 24 ✅ 3s ⏱️ Results for commit 58b2f19. ♻️ This comment has been updated with latest results. |
1 participant
Related issue 🛠
어떤 변경사항이 있었나요?
Work Description ✏️
작업 내용을 작성해주세요.
Uncompleted Tasks 😅
To Reviewers 📢
리뷰어가 알면 좋은 내용을 작성해주세요.